dev: Make the use of stdio FILE robust. #622
Conversation
There was a problem hiding this comment.
I'd like to think about this particular solution for a little bit. While I do agree with the new function additions, I am not so sure that we should override their names with macros in this manner. Something like this would make perfect sense in a C++ library, where overriding function names is idiomatic, and 100% working out of the box.
But we're stuck with C in here. And, for better or for worse, we've been having the goofy (but 100% working!) naming habit in which we overloaded existing function names with number suffixes: png_get_eXIf_1, png_create_write_struct_2, etc.
I, for one, would prefer leaving the old functions in place, perhaps with a deprecated attribute (to be removed eventually in some far-away future after the users migrate), and adding the new png_init_io_2, png_begin_read_from_stdio_2, etc.
In zlib they also use this practice. See, for example, deflateInit2.
(Yes, these API names are ugly 😝)
There is no choice. png_init_io and the simplified APIs don't work and are unsafe and there is a lot of code that uses them. They are good APIs even though the implementation (the ABI) is flawed. Therefore even though it's an ABI-change release this aspect of the API should remain the same. This is particularly true because it must be fread, fwrite and fflush which are passed to libpng, not something else. The OPs fix did what you just suggested but used C99 inline functions and massive unnecessary additions to the ABI while only addressing png_init_io! The inlines pass a function call across the boundary not the function itself but this seems to be mindbogglingly complex to me and it requires C99 while ISO-C allows functions to be passed as pointers! The function-like macros stop existing code and new code from doing the wrong thing; they hide the ABI but give an unchanged API which just does the right thing. What is more the result is much simpler and avoids adding yet more ABIs for no purpose. The whole thing about png_function_666 is that this was some half-baked attempt to pretend that the ABI wasn't being changed because it was "only" an addition. It's completely unnecessary to do that in an ABI-changing release and, anyway, it doesn't work because builds against the ABI-added 1.6 fail against prior DLL versions (and fail badly.) |
|
Change made using |
|
The pngimage.c fix is now #623 |
No problem. I'm using the command line to integrate PRs anyway. |
|
I still need a little more time to review this. Will be back tomorrow. |
ABI changes:
The three API functions png_init_io, png_begin_read_from_stdio and
png_image_write_to_stdio now require the host fread, fwrite and
fflush functions where appropriate in addition to the host FILE.
Internally libpng uses the provided functions for all operations on
the FILE; it does not use the implementations available when libpng
was built.
API changes:
The original APIs of the above three functions are now implemented
as function-like macros which automatically pass the correct ISO-C
functions. This ensures that there are no net API changes and that
the correct arguments are passed.
Build changes:
The read stdio functionality is now dependent on SEQUENTIAL_READ
rather than READ. This is done for clarity; the progressive read
mechanism does not use FILE.
Internal changes:
png_struct::io_ptr has been supplemented with png_struct::stdio_ptr
used when stdio is used directly by libpng for IO as opposed to
caller-provided callbacks.
Error checking has been added to detect mismatched use of the stdio
(png_init_io etc) API with the callback (png_set_read_fn,
png_set_write_fn APIs.) Changing from one API to the other
mid-stream should work but has not been tested and is not currently
a documented option.
The changes address an issue which is frequently encountered on
Microsoft Windows systems because Windows NT DLLs each have their own
ISO-C hosted environment. This means that a FILE from one DLL cannot be
used safely from a different DLL unless all access to the object is done
using functionality from the creating DLL. The problem is more general;
passing objects across DLL or other boundaries is frequently supported
but direct access to those objects' internal structure in the receiving
environment is not safe. Other such uses were address early on in the
development of libpng, this addresses the main, almost certainly, sole
remaining issue.
The idea of adding additional function pointers png_init_io was
suggested by github.com user pp383 in pull request pnggroup#208.
Signed-off-by: John Bowler <jbowler@acm.org>
jbowler
force-pushed
the
FILE-across-DLL-boundaries
branch
from
cb9b8cd
to
912c645
Compare
The now separate change to pngimage.c seems to have caused git to do an autosquash anyway, or maybe I did it (I did run --autosquash after fixing the merge conflicts.) Anyway it's back to a single commit now. |
ABI changes:
The three API functions png_init_io, png_begin_read_from_stdio and
png_image_write_to_stdio now require the host fread, fwrite and
fflush functions where appropriate in addition to the host FILE.
API changes:
The original APIs of the above three functions are now implemented
as function-like macros which automatically pass the correct ISO-C
functions. This ensures that there are no net API changes and that
the correct arguments are passed.
Build changes:
The read stdio functionality is now dependent on SEQUENTIAL_READ
rather than READ. This is done for clarity; the progressive read
mechanism does not use FILE.
Internal changes:
png_struct::io_ptr has been supplemented with png_struct::stdio_ptr
used when stdio is used directly by libpng for IO as opposed to
caller-provided callbacks.
The changes address an issue which is frequently encountered on
Microsoft Windows systems because Windows NT DLLs each have their own
ISO-C hosted environment. This means that a FILE from one DLL cannot be
used safely from a different DLL unless all access to the object is done
using functionality from the creating DLL. The problem is more general;
passing objects across DLL or other boundaries is frequently supported
but direct access to those objects' internal structure in the receiving
environment is not safe. Other such uses were address early on in the
development of libpng, this addresses the main, almost certainly, sole
remaining issue.
The idea of adding additional function pointers png_init_io was
suggested by github.com user pp383 in pull request #208.
Signed-off-by: John Bowler jbowler@acm.org